Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[extension/healthcheckv2extension] Refactor internal/status to pkg/status #35648

Merged
merged 18 commits into from
Oct 10, 2024

Conversation

blakerouse
Copy link
Contributor

@blakerouse blakerouse commented Oct 7, 2024

Description

Refactors the extension/healthcheckv2extension/internal/status into pkg/status.

This exposes the aggregator to be used by other extensions to gather component status information using the extension.StatusWatcher.

Link to tracking issue

Closes #34692

Testing

Being it was a refactor and all the same tests provided coverage, no additional testing was added.

Documentation

Added a README.md to the pkg/status to provide information on where this package can be used.

@blakerouse blakerouse requested review from jpkrohling, mwear and a team as code owners October 7, 2024 14:34
Copy link

linux-foundation-easycla bot commented Oct 7, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking on this refactor. I made a PR to this PR with a few additional updates that will be needed. In addition to those, this should have a changelog entry. Run make chlog-new to generate one and fill in the details.

pkg/status/README.md Outdated Show resolved Hide resolved
Versons, builder config, and metadata
@github-actions github-actions bot added the cmd/otelcontribcol otelcontribcol command label Oct 8, 2024
@blakerouse
Copy link
Contributor Author

@mwear Thanks for the cleanup PR. I have merged your PR and updated the README.md to the updated interface name.

@blakerouse
Copy link
Contributor Author

@mwear Forgot about the changelog. I have added that as well.

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @blakerouse!

cc: @bacherfl, @evan-bradley re: #35488.

pkg/status/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @blakerouse!

Overall this looks good to me, I just have comments on a few supplementary details.

.chloggen/pkg-status.yaml Outdated Show resolved Hide resolved
pkg/status/README.md Show resolved Hide resolved
pkg/status/metadata.yml Outdated Show resolved Hide resolved
@blakerouse
Copy link
Contributor Author

@evan-bradley I have update the requested items.

But running mage -C pkg/status generate resulted in:

make: *** No rule to make target `generate'.  Stop.

@mwear
Copy link
Member

mwear commented Oct 9, 2024

But running make -C pkg/status generate resulted in:

make: *** No rule to make target `generate'.  Stop.

I'm not sure why that doesn't work for a subdirectory, but you can run make generate from the root and will run for each contrib component. It's slow, but it will get the job done.

@blakerouse
Copy link
Contributor Author

@mwear I did do that. I didn't see it generate or do anything. Maybe there is nothing to generate?

@mwear
Copy link
Member

mwear commented Oct 9, 2024

@mwear I did do that. I didn't see it generate or do anything. Maybe there is nothing to generate?

It looks like there are two more changes needed:

Add a doc.go file with the following contents:

// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

//go:generate mdatagen metadata.yaml

package status // import "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/status"

Update metadata.yml to include a type, you can just copy this:

type: status

status:
  class: pkg
  stability:
    development: [extension]
  codeowners:
    active: [jpkrohling, mwear]

I found that if you cd into the pkg/status directory you can run mdatagen metadata.yml to generate for a specific component. make generate from the root should also work after these changes.

@blakerouse
Copy link
Contributor Author

@mwear @evan-bradley Thanks for the help on getting this ready. I think its ready now as the generation logic worked this time and filled in the correct information in the README.

@mwear
Copy link
Member

mwear commented Oct 9, 2024

The currently failing CI check says to run make generate-gh-issue-templates and commit the changes.

@blakerouse
Copy link
Contributor Author

@mwear Okay. maybe now its ready. 😄

@evan-bradley evan-bradley merged commit e9a428b into open-telemetry:main Oct 10, 2024
158 checks passed
@github-actions github-actions bot added this to the next release milestone Oct 10, 2024
@evan-bradley
Copy link
Contributor

Thank you @blakerouse! Your patience while working through the process of setting up a new module is appreciated.

@blakerouse
Copy link
Contributor Author

No problem! Glad to help.

sbylica-splunk pushed a commit to sbylica-splunk/opentelemetry-collector-contrib that referenced this pull request Dec 17, 2024
…atus (open-telemetry#35648)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Refactors the `extension/healthcheckv2extension/internal/status` into
`pkg/status`.

This exposes the aggregator to be used by other extensions to gather
component status information using the `extension.StatusWatcher`.

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue

Closes
open-telemetry#34692

<!--Describe what testing was performed and which tests were added.-->
#### Testing

Being it was a refactor and all the same tests provided coverage, no
additional testing was added.

<!--Describe the documentation added.-->
#### Documentation

Added a `README.md` to the `pkg/status` to provide information on where
this package can be used.

---------

Co-authored-by: Matthew Wear <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[extension/healthcheckv2] Ability to access the status.Aggregator from another extension
4 participants